Change f32::midpoint to upcast to f64 - #121062

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

r? @scottmcm

No need for a codegen test for rust code in core unless you're checking to ensure that a particular optimization is triggering on it -- most commonly it's for testing that particular instantiations simplify.

Just to try to (informally) prove it for myself:

• f32 has 1/8/23 bits, f64 has 1/11/52
• the add is never worse than a doubling, which will always fit in the exponent
• the division is never worse than half, which also fits in the exponent
• Thus nothing ever goes from finite to infinite
• Thus nothing ever goes from small to zero
• So the only special values are the ones from the inputs, for which the addition gives the correct behaviour and the division+cast preserves any special values.

Only question I'd have: I see tests in

That would be a bad choice for CPUs that have a single-precision FPU and emulate double precision. I don't know if we support any such targets, but such CPUs exist (some 32bit ARMs)

I don't know if we support any such targets, but such CPUs exist (some 32bit ARMs)

Seems like thumbv7em-none-eabihf only has a single FPU and does indeed generate calls to emulated double godbolt.

Not sure if we can do anything here about those targets, we don't have target cfg to differentiate them (we should maybe add one).

@scottmcm

Just to try to (informally) prove it for myself:

This looks correct

-- do you think those are sufficient? Do they cover potential double-roundings from quite-different-magnitude cases well enough. for example? (The a/2 + b/2 implementation is a single rounding, because halving doesn't round, assuming no underflow.)

No, I don't think that they test large differences in magnitude. I'll add tests for that.

@the8472

That would be a bad choice for CPUs that have a single-precision FPU and emulate double precision. I don't know if we support any such targets, but such CPUs exist (some 32bit ARMs)

Hm, I could fall back to the original implementation if we're on an embedded target tier 2 target. (If this is bad on a tier 3 target, I can add support if someone asks for specific targets before this PR gets merged). Would work? That would cover thumbv7em-none-eabihf for example.
I don't think any tier 1 target is missing double precision support.

Is there a reference that I can use to figure out which targets don't support double-precision floats? If not, I'll just use the original implementation on any target listed as Bare on the platform support page: https://doc.rust-lang.org/nightly/rustc/platform-support.html#tier-2-without-host-tools

Is there a reference that I can use to figure out which targets don't support double-precision floats?

No, not that I'm aware, this is the issue I alluded to in #121062 (comment), we don't have a target_... cfg for that.

One thing that could be done is to hack a cfg chain, like this, it should detect detect thumbv7em-none-eabihf and exclude armv7-unknown-linux-gnueabihf.

#[cfg(all(target_arch = "arm", target_pointer_width = "32", not(target_feature = "vfp2")))]

Do they cover potential double-roundings from quite-different-magnitude cases well enough. for example?

I don't think it's possible to get double rounding, as the precision of f64 is twice the precision of f32 and then some. Let's say the larger operand has 24 significant bits with significance 2⁵² to 2²⁹, while the smaller operand has 24 bits with significance 2²² to 2⁻¹. The addition results in rounding because of the bit at 2⁻¹, but that can only reach 2²³, which will not cause any double rounding in the conversion to f32.

Subtraction instead of addition shouldn't be able to break through the about 6 bits of leeway.

@tspiteri yes, with the proposed implementation, double rounding isn't possible, but it's good to add these tests anyways to ensure all future implementations correctly round the results.

@Urgau Thanks, I think I'll use that. 🙇

Have you benchmarked how this would affect an auto-vectorized loop of midpoint calculations? Upcasting to f64 means it can only have half the number of lanes which probably eats up some of the gains of the simpler algorithm.

@the8472 I checked godbolt, and there is no noticeable SIMD parallelization in either implementation. They are both equivalent to the scalar implementation. On x86_64 they do use SIMD registers, but this isn't because they are using SIMD for parallelization, but because x86/_64 only implents IEEE floats in SIMD, not in scalar ASM (they use 80-bit floats there). So we emit SIMD code to ensure that we are IEEE compliant.

I also did a simple benchmark to confirm my suspicions. You can find the code for the benchmark here: https://github.com/RustyYato/f32-perf-test
The results: my implementation is both more performant and more stable. The original implementation plays really poorly with the branch predictor. And doesn't outperform the upcast even in the best case scenarios

The benchmarks were run with criterion, picking the best results across both runs for each implementation

cargo bench
RUSTFLAGS='-C target-cpu=native' cargo bench

NOTE: ps = pico seconds (1/1000 of 1 ns)

For reference,
for a single operation upcasting to f64 performs between 780ps-840ps (depending on rustc flags/black box)
for a single operation f32 only performs between 850ps-1.060ns (depending on rustc flags/black box)
for 1M operations
numbers in the range 0-100 (the first branch of the original implementation)

• upcasting to f64: 6.5ms
• f32 only: 6.8ms

numbers in the range 0..f32::MAX (showing the worst cast of the original implementation)

• upcasting to f64: 6.6ms
• f32 only: 49.3ms 😱

> sudo dmidecode -t processor
Handle 0x0052, DMI type 4, 48 bytes
Processor Information
        Socket Designation: U3E1
        Type: Central Processor
        Family: Core i7
        Manufacturer: Intel(R) Corporation
        ID: 71 06 0B 00 FF FB EB BF
        Signature: Type 0, Family 6, Model 183, Stepping 1
        Flags:
                FPU (Floating-point unit on-chip)
                VME (Virtual mode extension)
                DE (Debugging extension)
                PSE (Page size extension)
                TSC (Time stamp counter)
                MSR (Model specific registers)
                PAE (Physical address extension)
                MCE (Machine check exception)
                CX8 (CMPXCHG8 instruction supported)
                APIC (On-chip APIC hardware supported)
                SEP (Fast system call)
                MTRR (Memory type range registers)
                PGE (Page global enable)
                MCA (Machine check architecture)
                CMOV (Conditional move instruction supported)
                PAT (Page attribute table)
                PSE-36 (36-bit page size extension)
                CLFSH (CLFLUSH instruction supported)
                DS (Debug store)
                ACPI (ACPI supported)
                MMX (MMX technology supported)
                FXSR (FXSAVE and FXSTOR instructions supported)
                SSE (Streaming SIMD extensions)
                SSE2 (Streaming SIMD extensions 2)
                SS (Self-snoop)
                HTT (Multi-threading)
                TM (Thermal monitor supported)
                PBE (Pending break enabled)
        Version: 13th Gen Intel(R) Core(TM) i7-13700K
        Voltage: 1.0 V
        External Clock: 100 MHz
        Max Speed: 8500 MHz
        Current Speed: 3400 MHz
        Status: Populated, Enabled
        Upgrade: Other
        L1 Cache Handle: 0x004F
        L2 Cache Handle: 0x0050
        L3 Cache Handle: 0x0051
        Serial Number: To Be Filled By O.E.M.
        Asset Tag: To Be Filled By O.E.M.
        Part Number: To Be Filled By O.E.M.
        Core Count: 16
        Core Enabled: 16
        Thread Count: 24
        Characteristics:
                64-bit capable
                Multi-Core
                Hardware Thread
                Execute Protection
                Enhanced Virtualization
                Power/Performance Control

> rustc --version --verbose
rustc 1.78.0-nightly (a4472498d 2024-02-15)
binary: rustc
commit-hash: a4472498d7e88041f6206faf4503eb1f246fd427
commit-date: 2024-02-15
host: x86_64-unknown-linux-gnu
release: 1.78.0-nightly
LLVM version: 18.1.0

I think this is pretty solid evidence that upcasting is better on current x86_64 hardware. But I don't have access to an ARM/powerpc/AARCH64 machine, so I can't verify other targets.

edit: just realized that I used inline(never) 🤦, so I reran the benchmarks without that. And to absolutely nobodies surprise, the core argument doesn't change. f64::from is is significantly better, but all numbers are a little faster. I don't really think the concrete numbers matter that much, just the differences, which didn't change as much. So I'm not going to edit in the fixed numbers. But the benchmark github repo is fixed.

If there are any deficiencies in my benchmark, I'm happy to run them again after those are fixed

The benchmarks use Vec<(f32, f32)>, I wouldn't expect those to optimize well because disentangling those into SIMD lanes is a bunch of extra work. Instead zipping two Vec<f32>s and doing the pairwise midpoints or doing midpoints over pairs of [f32; 8] is more likely to autovectorize. Bumping the CPU target to x86-64-v3 or v4 might make it more profitable too.

Ok, after that change, (switching from Vec<(f32, f32)> -> Vec<f32>, Vec<f32>), when the branch predictor is friendly, the original implementation is 14% faster, (5.27 ms vs 6.13 ms). This is significant, and something I missed before.
But when the branch predictor is not friendly the original implementation is still running at 48ms, while upcast is still running at 6.3ms.

I think stable performance is better than a 14% improvement. And if someone wanted to squeeze out that 10%, then they could copy out the original implementation.
Also when stable SIMD is available, I would assume that we could implement midpoint on f32x4 via the original implementation making this discussion about SIMD moot.

Bumping the CPU target to x86-64-v3 or v4 might make it more profitable too.

I'm not sure how to do that, if you could leave some instructions on how to do that, then I could try that out.

I'm not sure how to do that, if you could leave some instructions on how to do that, then I could try that out.

Prefix the benchmark command with RUSTFLAGS="-Ctarget-cpu=x86-64-v3"

But when the branch predictor is not friendly the original implementation is still running at 48ms, while upcast is still running at 6.3ms.

I don't do enough float number crunching to have an intuition how common it is for bulk data to fit into one of the branches vs. being spread between them. And and I think it'll only matter if you do bulk data crunching. If you only do some float computation every now and then it doesn't matter either way.

Anyway, now I think we have three questions:

• what if your CPU only has SP but not DP. This is handled for ARM now, but are there any others?
• what if performance differs between SP and DP (this was the case on some older x86 CPUs at least)
• autovectorization

This makes it complicated to assess whether it's worth it.

Prefix the benchmark command with RUSTFLAGS="-Ctarget-cpu=x86-64-v3"

I tried this and for most of the tests it didn't change performance, and for the ones it did only by ~1%. It seems the current implementation benefits from this more than the upcast implementation.

how common it is for bulk data to fit into one of the branches vs. being spread between them

I don't think there would be a large spread in most cases, but I would prefer not to have performance pitfalls that only occur in special cases if they are easy to avoid.

If you only do some float computation every now and then it doesn't matter either way.

In that case we shouldn't be considering that case, since either implementation will be fast enough.

what if your CPU only has SP but not DP. This is handled for ARM now, but are there any others?

I don't think any tier 1 targets do, for tier 2 or tier 3 targets, they can be added as special cases (or anyone using those targets can add those special cases if it matters). If the number of special cases gets large enough, maybe we can add a target specification for this instead.

what if performance differs between SP and DP (this was the case on some older x86 CPUs at least)

If the difference isn't that large (no more than 30% faster/slower), I think the branchless algorithm is still fine.
Also we could gate those architectures as well.

autovectorization

I would expect any code which needs performance front and center, won't rely on autovectorization. And since the upcast version is only 14% slower (on my machine at least), I wouldn't expect this to be a deal breaker. Also this was a very artificial benchmark, I would expect the difference in more realistic code to be much closer. Esp. since the vector registers are used by all float ops on x86(_64). It would make it harder for autovectorization to trigger.

This makes it complicated to assess whether it's worth it.

Yes, this was more involved that I expected 😄. No matter, I think we can sort this out.
In any case, this is a nightly function, we could try this out and if it is a net negative this change is easy to revert. So the opportunity cost is fairly low. This would also allow testing this on a wider variety of machines to get better data.

By chance, do you have a non-x86(_64) machine which you could test this out on?

I would expect any code which needs performance front and center, won't rely on autovectorization.

That's definitely not true. Writing architecture-specific SIMD code is a lot of work with all the features. Many people just beat the code into a shape that pleases LLVM and then rely on that. std does it too. We even have I-slow bug reports about those things regressing.

Yes, this was more involved that I expected 😄. No matter, I think we can sort this out.
By chance, do you have a non-x86(_64) machine which you could test this out on?

Yeah, I can test on an aarch64 cloud machine. And maybe some other x86 hardware, seems to be tricky enough to be worth it. What's your CPU?

If the difference isn't that large (no more than 30% faster/slower), I think the branchless algorithm is still fine.

Agner Fog's x86 instruction tables lists DIVSS having a latency of 9-17 and DIVSD 9-32. So the worst case is almost twice as much. But the latency is data-dependent and I think a power-of-two divisor is cheap and so won't experience worst-case latencies.

Icelake and various other *lake CPUs are listed as having an RThroughput (lower is better) of 3 for DIVSS vs. 4 for DIVSD. That'd be 33% slower in terms of throughput. So it could impact ILP which is a bit trickier to benchmark, you need the loop to do more work, with either some speculative execution or some independent data dependency chains.

What's your CPU?

13th Gen Intel(R) Core(TM) i7-13700K

Ok, after that change, (switching from Vec<(f32, f32)> -> Vec, Vec), when the branch predictor is friendly, the original implementation is 14% faster, (5.27 ms vs 6.13 ms). This is significant, and something I missed before.

The "hope auto vec" benches in your repo don't do that though. They still use Vec<(f32, f32)>, which i've checked on godbolt and they don't vectorize.

@the8472 my bad 🙃, I forgot to push my latest changes. They should be up now

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp.

Some changes occurred in coverage instrumentation.

cc @Zalathar

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in match lowering

cc @Nadrieril

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @TaKO8Ki

Sorry about the noise above, my bad. I've squashed everything into a single commit, given how small the change is I don't think there's much point in multiple commits for this.

@bors r+ rollup

📌 Commit 849c525 has been approved by the8472

It is now in the queue for this repository.

⌛ Testing commit 849c525 with merge 032af18...

bors what doing
@bors r- retry